Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: remove broken OAuth Application vacuuming & throttle OAuth Application registrations #30316

Conversation

ThisIsMissEm
Copy link
Contributor

In #24871, we introduced a mitigation for a potential database denial of service attack through registration of excessive volumes of OAuth Applications by vacuuming any Application that did not meet the following criteria:

  • any users signed up with the application
  • any valid access tokens or access grants
  • were not created from the “Development” tab

However, doing this fundamentally broke OAuth application registration for many developers, resulting in users have broken experiences due to the Application not knowing it had been deleted from the server with which it was registered.

This could happen if a user uses your application, then revokes access before another user starts using the same application. On the Application's side, you have a OAuth login request for their server, so you register an application for their server, and store the client_id, client_secret and server URL / domain, then the user revokes access to their account for your application (from their Mastodon server), then at some point in the future, a new user tries to perform an OAuth authorization with the same server, you knowing you've already registered an application with that server redirect them to the OAuth flow, however, the user is greeted with an error because your application has been silently deleted by the Mastodon server.

Additionally the vacuum process for Applications added a false sense of safety, since all you needed to do to prevent the vacuum from happening was to request client_credentials for your application, creating an access token that could never be deleted. A Security Vulnerability (GHSA-q7vj-88hq-gjmr) was filed for this issue about this 5 months ago, however no resolution was found, I'm disclosing that "vulnerability" here now, since it has now become fairly common knowledge amongst more experienced OAuth application developers.

For now, I've added a rate limit to the POST /api/v1/apps endpoint to try to curb any abuse, however I'm not sure if I have the configuration values right; we may need to adjust up or down.

Related issue: #27740

@ThisIsMissEm
Copy link
Contributor Author

Example of the OAuth Application vacuuming being circumvented in the wild: https://github.com/neodb-social/neodb/pull/565/files#diff-532f87bbb7a6c0332c62fc4429bbc52bb6d6685e8650ddf3c50dc1a16dba73cdR431-R440 (we also do the same with IFTAS FediCheck)

@ThisIsMissEm
Copy link
Contributor Author

For why I chose 5 requests per 30 minutes, it's because that's the same limit used for throttling account sign-ups by the API. Arguably, that limit could maybe be lower (1-2 successful per 10 minutes)

@ThisIsMissEm
Copy link
Contributor Author

This may be incompatible with public clients and we may actually want vacuuming of public clients with no access tokens or access grants (but NOT vacuuming of confidential clients with no access tokens or access grants).

@ThisIsMissEm
Copy link
Contributor Author

ThisIsMissEm commented May 17, 2024

If we implement public clients ( #30329 ), then we could keep the vacuum for just public clients, since those would expire regularly.

To avoid the resulting proliferation of dead client identifiers, an authorization server MAY decide to expire registrations for existing clients meeting certain criteria after a period of time has elapsed.

We could also make a breaking change in Mastodon 5.0 to expire all clients after a year, meaning folks would need to reauthenticate once a year (or we provide a mechanism for an application to extend the expiry of their clients like POST /api/v1/apps/renew that adds an addition X time to the client expiry; or perhaps rotates the client secret as well).

We could still keep clients that are owned by users to not expire (though this has potential safety issues)

The problem at the moment is that the vacuum process is not deterministic, from what I can tell, since it's tethered to the number of access tokens & access grants, which at any point in time for a OAuth Application may drop to zero momentarily.

@ClearlyClaire
Copy link
Contributor

This may be incompatible with public clients

What do you mean by “incompatible”?

@ThisIsMissEm
Copy link
Contributor Author

This may be incompatible with public clients

What do you mean by “incompatible”?

@ClearlyClaire see the reply after that. Basically public clients you may want to actually vacuum, but confidential clients should never be vacuumed.

I'm actually working on this IETF I-D for solving the public client use-case too: https://drafts.aaronpk.com/draft-parecki-oauth-client-id-metadata-document/draft-parecki-oauth-client-id-metadata-document.html

This basically removes the need for dynamic client registration in majority of use-cases.

@ClearlyClaire
Copy link
Contributor

So this isn't an incompatibility, but just that there is no harm doing this for public clients?

@ThisIsMissEm
Copy link
Contributor Author

So this isn't an incompatibility, but just that there is no harm doing this for public clients?

I think so, yeah; though I'd rather have public clients use URI based client_id's, since that mitigates this entire problem.

@ClearlyClaire
Copy link
Contributor

Looks ok overall, though I'm not sure about the rate limits. Looking at mastodon.social's logs, we do regularly get > 5 registrations per 30 minutes per IP. Though it's possible some of those are malicious or otherwise abusive in the first place.

config/initializers/rack_attack.rb Outdated Show resolved Hide resolved
@ClearlyClaire ClearlyClaire added the to backport PR needed to be backported label May 29, 2024
From 5 requests every 30 minute to 5 requests every 10 minutes
@ClearlyClaire ClearlyClaire requested a review from a team May 29, 2024 13:36
@ClearlyClaire ClearlyClaire added this pull request to the merge queue May 29, 2024
Merged via the queue into mastodon:main with commit d20a5c3 May 29, 2024
30 checks passed
ClearlyClaire added a commit that referenced this pull request May 29, 2024
…cation registrations (#30316)

Co-authored-by: Claire <claire.github-309c@sitedethib.com>
ClearlyClaire added a commit that referenced this pull request May 29, 2024
…cation registrations (#30316)

Co-authored-by: Claire <claire.github-309c@sitedethib.com>
ClearlyClaire added a commit that referenced this pull request May 29, 2024
…cation registrations (#30316)

Co-authored-by: Claire <claire.github-309c@sitedethib.com>
@ThisIsMissEm ThisIsMissEm deleted the fix/remove-broken-oauth-application-vacuum-worker branch May 29, 2024 23:27
kmycode pushed a commit to kmycode/mastodon that referenced this pull request May 30, 2024
…cation registrations (mastodon#30316)

Co-authored-by: Claire <claire.github-309c@sitedethib.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to backport PR needed to be backported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants